fix(display): close the video writer when the audio writer's close throws - #291

The issue

recordScreenAndAudioToFiles closed its two writers in sequence:

audioWritten = await audioOut.close();
await videoOut.close();

If the audio close throws, the video close never runs.

That is not a hypothetical ordering nit. M4aFileWriter.finalize() has four throw sites, all reached after the recording has succeeded:

await this.push(moov);                          // rethrows a held stream error
await new Promise(... this.stream.end(...));    // rejects on a flush failure
const handle = await open(this.path, 'r+');     // EACCES / ENOENT if the file moved
await handle.write(u32(...), 0, 4, ...);        // ENOSPC patching the mdat header

The last pair is the notable one: the writer reopens the finished .m4a to patch mdat's length. A disk that fills during a long recording, or a temp directory reaped underneath it, fails there once every frame is already safely on disk.

What it actually costs

A leaked file descriptor. Without close(), stream.end() is never called, and Node does not release descriptors on garbage collection, so it lives until the process exits.

A silently held error. AnnexBFileWriter keeps the first stream error and surfaces it from write() or close(). With close() skipped, nobody ever reads it.

That second part got quieter as a side effect of #289. Before AnnexBFileWriter existed, an abandoned stream that errored later produced an uncaught 'error' and was at least loud. Now the permanent listener holds it and no one looks. Noisy to silent.

What it does not cost

Worth stating plainly, because an earlier draft of this analysis got it wrong: the video file is intact. Node's write queue drains autonomously once chunks are handed to write(), the recording loop awaits every videoOut.write(), and Annex-B is an elementary stream with no index or footer to finalize — unlike .m4a's mdat patch. A file ending after the last written frame is valid and playable. Verified by writing 15 MB through AnnexBFileWriter and never calling close():

expected 15728640 bytes, on disk 15728640
COMPLETE — no truncation

The caller does not get a misleading result either: the function rejects, so no result object with framesWritten is ever returned.

So: one leaked fd plus an unread error, on a path that has already failed. Genuinely minor — worth fixing because it is two lines, not because it is dangerous.

The fix

try {
  audioWritten = await audioOut.close();
} finally {
  await videoOut.close();
}

The audio error still propagates when the video close succeeds. If both throw, the video error supersedes via standard finally behaviour — an acceptable edge on a path that has already failed, and the same judgement reached in review of the earlier PRs.

Tests

A unit test stubs M4aFileWriter.close() to throw and counts AnnexBFileWriter.close() calls, using the repo's existing mockImport helper. The mock merges over the real exports, so ScreenStreamCapture stays intact and the capture path runs for real against a stub DisplayService.

Mutation-checked by reverting to sequential closes in the compiled output:

✔ stops both captures when the audio file cannot be created
✖ closes the video writer even when closing the audio writer throws   actual: 0

Only the new test fails, with the intended assertion.

Not verified against hardware: this path needs an iOS 27 device (npm run test:display). What is verified is the close ordering that was wrong.

Scope

This is the third and last of the resource-lifetime findings from reviewing the DisplayService recording paths, after #289 (uncaught write-stream errors) and #290 (captures stranded when the output file cannot be created).

One unrelated finding remains open and is deliberately not included here: parseRtpPacket ignores the RTP padding bit (data[0] & 0x20), so padding octets would be handed downstream as payload. It is spec conformance with no evidence the device sets the bit, and it belongs in its own PR rather than inside one about file descriptors.